-
Notifications
You must be signed in to change notification settings - Fork 15.1k
clang tidy: add header ignore option to include duplicate files #167046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Lakshdeep Singh (lakshsidhu04) ChangesAdded an option to enable users to include duplicate headers which match with certain regexes Full diff: https://github.com/llvm/llvm-project/pull/167046.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
index a0e7ac19ab2d5..bf0d9a7598679 100644
--- a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
@@ -107,7 +107,7 @@ class CyclicDependencyCallbacks : public PPCallbacks {
Check.diag(Loc, "direct self-inclusion of header file '%0'") << FileName;
return;
}
-
+
Check.diag(Loc, "circular header file dependency detected while including "
"'%0', please check the include path")
<< FileName;
diff --git a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
index 0237c057afed5..5ed783bcef824 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
@@ -11,7 +11,12 @@
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Regex.h"
+#include "llvm/Support/Path.h"
+#include "../utils/OptionsUtils.h"
+#include "llvm/ADT/StringExtras.h"
#include <memory>
+#include <vector>
namespace clang::tidy::readability {
@@ -33,12 +38,9 @@ using FileList = SmallVector<StringRef>;
class DuplicateIncludeCallbacks : public PPCallbacks {
public:
DuplicateIncludeCallbacks(DuplicateIncludeCheck &Check,
- const SourceManager &SM)
- : Check(Check), SM(SM) {
- // The main file doesn't participate in the FileChanged notification.
- Files.emplace_back();
- }
-
+ const SourceManager &SM,
+ const std::vector<std::string> &AllowedStrings);
+
void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
FileID PrevFID) override;
@@ -62,14 +64,28 @@ class DuplicateIncludeCallbacks : public PPCallbacks {
SmallVector<FileList> Files;
DuplicateIncludeCheck &Check;
const SourceManager &SM;
+ std::vector<llvm::Regex> AllowedDuplicateRegex;
+
+ bool IsAllowedDuplicateInclude(StringRef TokenName, OptionalFileEntryRef File,
+ StringRef RelativePath);
};
} // namespace
-void DuplicateIncludeCallbacks::FileChanged(SourceLocation Loc,
- FileChangeReason Reason,
- SrcMgr::CharacteristicKind FileType,
- FileID PrevFID) {
+DuplicateIncludeCallbacks::DuplicateIncludeCallbacks(
+ DuplicateIncludeCheck &Check, const SourceManager &SM,
+ const std::vector<std::string> &AllowedStrings) : Check(Check), SM(SM) {
+ // The main file doesn't participate in the FileChanged notification.
+ Files.emplace_back();
+ AllowedDuplicateRegex.reserve(AllowedStrings.size());
+ for (const std::string &str : AllowedStrings) {
+ AllowedDuplicateRegex.emplace_back(str);
+ }
+}
+
+void DuplicateIncludeCallbacks::FileChanged(
+ SourceLocation Loc, FileChangeReason Reason,
+ SrcMgr::CharacteristicKind FileType, FileID PrevFID) {
if (Reason == EnterFile)
Files.emplace_back();
else if (Reason == ExitFile)
@@ -85,6 +101,14 @@ void DuplicateIncludeCallbacks::InclusionDirective(
if (FilenameRange.getBegin().isMacroID() ||
FilenameRange.getEnd().isMacroID())
return;
+
+ // if duplicate allowed, record and return
+ if(IsAllowedDuplicateInclude(FileName, File, RelativePath))
+ {
+ Files.back().push_back(FileName);
+ return;
+ }
+
if (llvm::is_contained(Files.back(), FileName)) {
// We want to delete the entire line, so make sure that [Start,End] covers
// everything.
@@ -109,9 +133,54 @@ void DuplicateIncludeCallbacks::MacroUndefined(const Token &MacroNameTok,
Files.back().clear();
}
+bool DuplicateIncludeCallbacks::IsAllowedDuplicateInclude(StringRef TokenName,
+ OptionalFileEntryRef File,
+ StringRef RelativePath) {
+ SmallVector<StringRef, 3> matchArguments;
+ matchArguments.push_back(TokenName);
+
+ if (!RelativePath.empty())
+ matchArguments.push_back(llvm::sys::path::filename(RelativePath));
+
+ if (File) {
+ StringRef RealPath = File->getFileEntry().tryGetRealPathName();
+ if (!RealPath.empty())
+ matchArguments.push_back(llvm::sys::path::filename(RealPath));
+ }
+
+ // try to match with each regex
+ for (const llvm::Regex ® : AllowedDuplicateRegex) {
+ for (StringRef arg : matchArguments) {
+ if (reg.match(arg))
+ return true;
+ }
+ }
+ return false;
+}
+
+DuplicateIncludeCheck::DuplicateIncludeCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {
+ std::string Raw = Options.get("AllowedDuplicateIncludes", "").str();
+ if (!Raw.empty()) {
+ SmallVector<StringRef, 4> StringParts;
+ StringRef(Raw).split(StringParts, ',', -1, false);
+
+ for (StringRef Part : StringParts) {
+ Part = Part.trim();
+ if (!Part.empty())
+ AllowedDuplicateIncludes.push_back(Part.str());
+ }
+ }
+}
+
void DuplicateIncludeCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
- PP->addPPCallbacks(std::make_unique<DuplicateIncludeCallbacks>(*this, SM));
+ PP->addPPCallbacks(std::make_unique<DuplicateIncludeCallbacks>(*this, SM, AllowedDuplicateIncludes));
}
+void DuplicateIncludeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "AllowedDuplicateIncludes",
+ llvm::join(AllowedDuplicateIncludes, ","));
+}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
index 297999cf4f921..688c2ad4d30f1 100644
--- a/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
@@ -10,7 +10,8 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATE_INCLUDE_CHECK_H
#include "../ClangTidyCheck.h"
-
+#include <vector>
+#include <string>
namespace clang::tidy::readability {
/// \brief Find and remove duplicate #include directives.
@@ -19,11 +20,14 @@ namespace clang::tidy::readability {
/// directives between them are analyzed.
class DuplicateIncludeCheck : public ClangTidyCheck {
public:
- DuplicateIncludeCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ DuplicateIncludeCheck(StringRef Name, ClangTidyContext *Context);
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+private:
+ std::vector<std::string> AllowedDuplicateIncludes;
};
} // namespace clang::tidy::readability
|
|
Hi, i am relatively new to open source. Can you tell me if I need to make documentation changes if any? |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/other.h clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/pack_begin.h clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/pack_end.h clang-tools-extra/test/clang-tidy/checkers/readability/duplicate-include-allowed.cpp clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
index bf0d9a759..a0e7ac19a 100644
--- a/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
@@ -107,7 +107,7 @@ public:
Check.diag(Loc, "direct self-inclusion of header file '%0'") << FileName;
return;
}
-
+
Check.diag(Loc, "circular header file dependency detected while including "
"'%0', please check the include path")
<< FileName;
|
Hi, yes: you need to add 1)release notes, 2)tests and 3)documentation about new option. General advice: look at how other people do PRs that got merged - there you can find answers by yourself. Please try to make robust test cases and cover all edge-cases that you can think of! |
You can test this locally with the following command:git diff -U0 origin/main...HEAD -- clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h |
python3 clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \
-path build -p1 -quietView the output from clang-tidy here. |
| Option: ``AllowedDuplicateIncludes`` | ||
| ------------------------------------ | ||
|
|
||
| Headers listed in this option are exempt from warnings. For example: | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| -config='{CheckOptions: [{key: readability-duplicate-include.AllowedDuplicateIncludes, value: "pack_begin.h,pack_end.h"}]}' | ||
|
|
||
| This allows regex matches with ``pack_begin.h`` and ``pack_end.h`` to be included multiple times | ||
| without triggering diagnostics. | ||
|
|
||
| Notes | ||
| ----- | ||
|
|
||
| - Only direct includes in the current translation unit are checked. | ||
| - Useful for removing redundant includes and improving compile times in large codebases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong option formatting, please see how other documents with options are formatted
| Option: ``AllowedDuplicateIncludes`` | ||
| ------------------------------------ | ||
|
|
||
| Headers listed in this option are exempt from warnings. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add default value
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/duplicate-include-allowed/other.h
Outdated
Show resolved
Hide resolved
| #include "assertion.h" | ||
| // ...code with assertions disabled | ||
|
|
||
| Option: ``AllowedDuplicateIncludes`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name it IgnoreHeaders, see https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html.
Just copy documentation from there
| std::string Raw = Options.get("AllowedDuplicateIncludes", "").str(); | ||
| if (!Raw.empty()) { | ||
| SmallVector<StringRef, 4> StringParts; | ||
| StringRef(Raw).split(StringParts, ',', -1, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be semicolon-separated, see https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html#cmdoption-arg-IgnoreHeaders.
| if (!RelativePath.empty()) | ||
| matchArguments.push_back(llvm::sys::path::filename(RelativePath)); | ||
|
|
||
| if (File) { | ||
| StringRef RealPath = File->getFileEntry().tryGetRealPathName(); | ||
| if (!RealPath.empty()) | ||
| matchArguments.push_back(llvm::sys::path::filename(RealPath)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need 3 separate matchArguments to match?
To my understanding when we encounter:
#include "pack_begin.h"
#include "pack_begin.h" We only need to match only header name, what are other 2 matchArguments?
0658b02 to
b611038
Compare
Added an option to enable users to include duplicate headers which match with certain regexes